Skip to content

Refactor Users#index to paginate and filter on the backend. Closes #266 #272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jan 26, 2018
Merged

Refactor Users#index to paginate and filter on the backend. Closes #266 #272

merged 8 commits into from
Jan 26, 2018

Conversation

@knoopx
Copy link
Contributor

@knoopx knoopx commented Oct 18, 2017 •
edited
Loading

  • Results are now paginated using kaminari on the server side
  • Filtering and sorting is performed now on the server side using ransack, which was already part of the bundle (activeadmin depends on it) and because, in my opinion, elasticsearch was kind of overkill for this specific case
  • Removed angular completely (couldn't find any other usage within the app)

@sauloperez
Copy link
Collaborator

sauloperez commented Oct 18, 2017 •
edited
Loading

Welcome to TimeOverflow @knoopx ! keep it coming, it looks pretty good so far 🎉 !

enricostano
enricostano reviewed Oct 18, 2017
View reviewed changes
app/helpers/users_helper.rb
}
end.to_json.html_safe
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sauloperez
Copy link
Collaborator

I thought the login had some piece angular too

@sseerrggii
Copy link
Contributor

sseerrggii commented Oct 20, 2017 •
edited by sauloperez
Loading

Thanks!! I see 3 things after testing:

  • The top nav dropdown doesn't open (see gif)
  • Sorting users by balance doesn't work either (see gif)
  • We need to add a search button like we have in offers. Until now the field was fuzzy filtering, now this behaviour change might be confusing for users in terms of UX.

peek 20-10-2017 16-11

@markets
Copy link
Collaborator

markets commented Oct 20, 2017

Hi guys 👋, about:

The top nav dropdown doesn't open

I think this can be related: 278f0d4

Thanks a lot @knoopx for this PR!

@sauloperez sauloperez mentioned this pull request Oct 21, 2017
Closed
3 tasks
@sseerrggii
Copy link
Contributor

👏👏👏 @knoopx

Could it be possible to sort by balance? administrators use it... It just did not work well

@sseerrggii
Copy link
Contributor

Could it be possible to sort by balance? administrators use it... It just did not work well

@sauloperez 🙏

@sauloperez sauloperez self-assigned this Jan 11, 2018
sauloperez
sauloperez reviewed Jan 18, 2018
View reviewed changes
app/helpers/users_helper.rb
valid_email: user.has_valid_email?
}
end.to_json.html_safe
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure we also get rid of the private methods this might use.

@sauloperez
Copy link
Collaborator

I'm taking this over. Sorry @knoopx, we need to finish it up to move on to new things.

@sauloperez
Copy link
Collaborator

Ok I got why sorting by balance doesn't work out of the box. It's a column on the accounts table, so an inner join between users, members and account is mandatory.

@sauloperez
Copy link
Collaborator

sauloperez commented Jan 25, 2018 •
edited
Loading

Done @sseerrggii !

sort

@enricostano
Copy link
Contributor

@sseerrggii @sauloperez do the changes in locales files will need some processing in localeapp?

@sauloperez
Copy link
Collaborator

Given the nature of the change, I guess not, but we'll take a look.

@sseerrggii
Copy link
Contributor

fireshot capture 8 - banco de tiempo local - http___local timeoverflow org_3000_members
A scroll tab has appeared, in chrome

@sauloperez
Copy link
Collaborator

🙈 I'll take a look.

@sauloperez
Don't show useless horizontal scrollbar in destkop
c7a2d31 
This removes the scroll bar that shows up in the users list, which you
can't move at all.
@sauloperez
Copy link
Collaborator

Done @sseerrggii. Although was a preexistent issue I managed to fix easily. Can you take another look?

@sauloperez
Copy link
Collaborator

Tested and working. Moving on!

@sauloperez sauloperez merged commit 79e963a into coopdevs:develop Jan 26, 2018
@sauloperez sauloperez mentioned this pull request Jan 30, 2018
Merged
@sseerrggii sseerrggii mentioned this pull request Apr 6, 2018
Closed
@sseerrggii sseerrggii mentioned this pull request May 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sauloperez sauloperez sauloperez left review comments

+1 more reviewer

@enricostano enricostano enricostano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sauloperez sauloperez

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@knoopx @sauloperez @sseerrggii @markets @enricostano